Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support java.util.Optional<T> passing to @QueryParam/@RestQuery for RestСlient #45672

Merged
merged 1 commit into from
Jan 17, 2025
Merged

Support java.util.Optional<T> passing to @QueryParam/@RestQuery for RestСlient #45672

merged 1 commit into from
Jan 17, 2025
+90 −0

Conversation

lasteris
Copy link
Contributor

@lasteris lasteris commented Jan 17, 2025 •
edited by geoand
Loading

@quarkus-bot quarkus-bot
Copy link

quarkus-bot bot commented Jan 17, 2025

Thanks for your pull request!

Your pull request does not follow our editorial rules. Could you have a look?

  • description should not be empty, describe your intent or provide links to the issues this PR is fixing (using Fixes #NNNNN) or changelogs

This message is automatically generated by a bot.

geoand
geoand reviewed Jan 17, 2025
View reviewed changes
...client/deployment/src/test/java/io/quarkus/rest/client/reactive/OptionalQueryParamsTest.java Outdated
Comment on lines 50 to 53
Map.Entry<String, List<String>> optionalParam = uriInfo.getQueryParameters().entrySet().iterator()
.next();

return optionalParam.getValue().get(0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a huge fan of this, I would rather we return some string like key1=value1,value2/key2=value...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a huge fan of this, I would rather we return some string like key1=value1,value2/key2=value...

No problem) I just decided that it would be more obvious that we are testing not a Map, but an Optional)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a huge fan of this, I would rather we return some string like key1=value1,value2/key2=value...

fixed, pushed changes

@lasteris lasteris force-pushed the docs-rest-client-fix-link branch from e0722b2 to 4036dab Compare January 17, 2025 07:50
Ladicek
Ladicek reviewed Jan 17, 2025
View reviewed changes
.../src/main/java/io/quarkus/jaxrs/client/reactive/deployment/JaxrsClientReactiveProcessor.java
} else if (isOptional(type, index)) {
if (type.kind() == PARAMETERIZED_TYPE) {
Type paramType = type.asParameterizedType().arguments().get(0);
if ((paramType.kind() == CLASS) || (paramType.kind() == PARAMETERIZED_TYPE)) {
Copy link
Contributor

@Ladicek Ladicek Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically the type argument of Optional could also be an array.

EDIT: And a wildcard. Let's not get too wild with type variables :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, i am tried to understand these mechanisms for hours, but still having some troubles :)
Do you mean, that i can just drop

if ((paramType.kind() == CLASS) || (paramType.kind() == PARAMETERIZED_TYPE)) {

and assign componentType directly to componentType = paramType.name().toString(); and that's safe ?
or i need to limit support of parameters type (exclude arrays and etc)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I honestly have no idea, I'm not familiar with this code at all. Just pointed out something that I saw while briefly going through this PR for no particular reason :-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. At some point we might need to handle all the possible types, but we can keep that change for another day

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 17, 2025 •
edited
Loading

🙈 The PR is closed and the preview is expired.

@quarkus-bot quarkus-bot
Copy link

quarkus-bot bot commented Jan 17, 2025

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 4036dab.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

@quarkus-bot quarkus-bot
Copy link

quarkus-bot bot commented Jan 17, 2025 •
edited by github-actions bot
Loading

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 4036dab.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

Flaky tests - Develocity

⚙️ Native Tests - HTTP

📦 integration-tests/rest-client-reactive

✖ io.quarkus.it.rest.client.BasicTestIT.shouldCreateClientSpans - History

  • expected: <1> but was: <2> - org.opentest4j.AssertionFailedError
org.opentest4j.AssertionFailedError: expected: <1> but was: <2>
	at io.quarkus.it.rest.client.BasicTest.shouldCreateClientSpans(BasicTest.java:216)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)
	at io.quarkus.test.junit.QuarkusTestExtension.interceptTestMethod(QuarkusTestExtension.java:789)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

@geoand geoand merged commit e75b0fd into quarkusio:main Jan 17, 2025
31 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.19 - main milestone Jan 17, 2025
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Jan 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ladicek Ladicek Ladicek left review comments

@geoand geoand geoand approved these changes

Assignees
No one assigned
Labels
area/documentation area/rest area/rest-client kind/enhancement New feature or request triage/flaky-test
Projects
None yet
Milestone
3.19 - main
Development

Successfully merging this pull request may close these issues.

Support optional @QueryParam on Rest client
3 participants
@lasteris @Ladicek @geoand